Use modelName instead of typeKey when available, but preserve existing ID generation behavior #64
Conversation
|
The integration tests work with PhantomJS 2.0 only, not 1.9. I thought from reading the So, the CI build is going to fail. I'll look into getting PhantomJS 2.0 available for the build. |
|
👍 |
|
👍 Looks good to me |
|
Tested on https://github.com/broerse/ember-cli-blog and it works fine! |
|
@rsutphin looks good but can we use the new terminology and do |
|
@fsmanuel, I deliberately used a different term from either
Do you think not calling it something that includes |
Happy path only. Verifies only that records in PouchDB can be read into / written from ember-data models. Includes manually downloading PhantomJS 2.0 for Travis — the integration tests don't work with PhantomJS 1.9.
This removes scads of deprecation warnings with ember-data 1.0-beta18 and later while preserving access to data created with current versions of ember-pouch. See pouchdb-community#63 for a discussion.
rsutphin
force-pushed
the
record_type_name
branch
from
5e79a11 to
8aa35df
Compare
|
Squashed the various CI-fixing commits. |
| @@ -131,7 +133,8 @@ export default DS.RESTAdapter.extend({ | |||
|
|
|||
| _recordToData: function (store, type, record) { | |||
| var data = {}; | |||
| var serializer = store.serializerFor(type.typeKey); | |||
| var recordTypeName = this.getRecordTypeName(type); | |||
| var serializer = store.serializerFor(recordTypeName); | |||
There was a problem hiding this comment.
I think we get a problem here when ED transitions to dasherized types.
There was a problem hiding this comment.
That's a good point. I initially used type.modelName || type.typeKey instead of getRecordTypeName in this method for that reason. But the serialized data comes back with a camelized key (see line 155 below), and I figured it wouldn't hurt to use the camelized version in both places.
ED has supported either dasherized or camelized names for lookups (modelFor, serializerFor, etc) for a while — have you seen something that indicates that it is going to stop supporting camelized ones at some point?
There was a problem hiding this comment.
Thinking about this some more — these uses are conceptually different from recordTypeName, so it's probably best not to use it here. E.g., if you override getRecordTypeName to provide your own scheme, the serializer is still going to return the data with a camelized key. And if the serializer changes in the future, it shouldn't affect the type name used in the record IDs. So I'll change this.
There was a problem hiding this comment.
Addressed in 4b33181.
|
@rsutphin i see your point. What do you think about two functions: modelNameFromType(type) {
if (type.modelName) {
return type.modelName;
} else {
// This branch can be removed when the library drops support for
// ember-data 1.0-beta17 and earlier.
return type.typeKey;
}
}recordTypeName(type) {
if (this.get('useLegacyTypes')) {
return camelize(this.modelNameFromType(type));
}
return this.modelNameFromType(type);
}If we call it a breaking change new users can use the dasherized types and old users should set |
|
@fsmanuel, we could do that, but it seems unnecessary. Can you help me understand why you think this complexity (including a new option to document and support, communicating the breaking change to all existing users, etc.) is worth it just to use dasherized names in document IDs instead of camelized ones? The specific details of the document IDs aren't exposed to regular users of the adapter, so the risk of confusion with what ember-data does is very low. I agree that the interaction with the serializer needs to be addressed; I'll push something up shortly to handle that. |
|
Nice work! Love the cross-version solution and the new tests. And nice hack to get PhantomJS 2.0 working; I didn't realize it was possible to run it in Travis. :) Thanks for all the input, everyone! I'll publish a patch version today. |
|
2.0.1 published and changelog in the README updated. Thanks again everyone! |
N.b., the new test does not pass on ember-data 1.0-beta19 or 19.1. The problem appears to be that the relationship returned by eachRelationship has the name of the associated type instead of the DS.Model object. It works fine on older versions (tested beta17 and 18). Have not tried in a real app yet to see if this is a test artifact or an ember-data bug.
N.b., the new test does not pass on ember-data 1.0-beta19 or 19.1. The problem appears to be that the relationship returned by eachRelationship has the name of the associated type instead of the DS.Model object. It works fine on older versions (tested beta17 and 18). Have not tried in a real app yet to see if this is a test artifact or an ember-data bug.
This is my suggestion for #62, based on my desire to prevent all existing ember-pouch users from having to do a data migration as discussed in #63.
It also includes a set of very basic integration tests. If this feature doesn't get integrated, they should still be usable on their own. (I wrote them to verify that this change did preserve the prior behavior.)